Optimization: Avoid quadratic behavior in IdMaker.make_unique_parameterset_ids#14739
Optimization: Avoid quadratic behavior in IdMaker.make_unique_parameterset_ids#14739NoxiousTab wants to merge 6 commits into
Conversation
make_unique_parameterset_ids rebuilt set(resolved_ids) from scratch on every candidate suffix it tried while de-duplicating parametrize ids. For parametrizations with many colliding ids this made the method scale O(n^2) with the number of parameter sets. Build the set of in-use ids once and keep it updated incrementally instead, bringing this down to linear time. Confirmed with a manual benchmark, runtime dropped from ~0.64s to ~0.01s, and the doubling ratio changed from ~4x (quadratic) to ~2x (linear).
psf-chronographer
Bot
added
the
bot:chronographer:provided
RonnyPfannschmidt
left a comment
RonnyPfannschmidt
left a comment
There was a problem hiding this comment.
Request changes
The cached-set idea is good (the set(resolved_ids) rebuild really is quadratic on heavy collisions), but the parallel set is only half-updated: new ids are added, old ids are never discarded. That changes node ids vs main, not just performance.
Required fix
When replacing resolved_ids[index], keep used_ids equivalent to the ids still present in the list (e.g. track a remaining Counter and discard the old id when its count hits 0). See the inline comment for a sketch.
Required test
Please add a regression test in testing/python/metafunc.py (near the other test_idmaker_* cases) that fails on this PR branch and passes on main / after the fix. Use the simplified counter-example from the inline comment, expressed via IdMaker + explicit ids:
def test_idmaker_unique_ids_frees_renamed_away_ids(self) -> None:
"""Suffix collision must see ids freed after all duplicates are renamed.
Simplified resolved-id sequence: ["a10", "a10"] + ["a"] * 11
After renaming both "a10" entries, "a10" must be available again for
the 11th "a". A set that only .add()s new ids and never discards old
ones incorrectly yields "a11" as the last id.
"""
ids = ["a10", "a10"] + ["a"] * 11
result = IdMaker(
("x",),
[pytest.param(i) for i in range(len(ids))],
None,
ids,
None,
None,
).make_unique_parameterset_ids()
assert result == [
"a10_0",
"a10_1",
"a0",
"a1",
"a2",
"a3",
"a4",
"a5",
"a6",
"a7",
"a8",
"a9",
"a10",
]Please do not land the optimization without that behavior lock-in.
| resolved_ids[index] = new_id | ||
| used_ids.add(new_id) |
There was a problem hiding this comment.
This only half-updates the parallel set: resolved_ids[index] is replaced, but the old id is never removed from used_ids when it no longer occurs in the list.
That is not just an invariant smell — it changes assigned ids whenever a later candidate equals a fully-renamed-away earlier id.
Simplified counter-example (same algorithm as this loop):
# input
["a10", "a10"] + ["a"] * 11
# main today (rebuild set(resolved_ids) each probe)
["a10_0", "a10_1", "a0", "a1", "a2", "a3", "a4", "a5", "a6", "a7", "a8", "a9", "a10"]
# this PR (stale "a10" left in used_ids)
["a10_0", "a10_1", "a0", "a1", "a2", "a3", "a4", "a5", "a6", "a7", "a8", "a9", "a11"] # <-- last id wrongAfter both "a10" entries are renamed, main frees "a10" for the 11th "a". Here "a10" stays reserved forever, so the last id becomes "a11".
Please keep used_ids in sync, e.g. with a remaining-count:
used_ids = set(resolved_ids)
remaining = Counter(resolved_ids)
...
remaining[id] -= 1
if remaining[id] == 0:
used_ids.discard(id)
resolved_ids[index] = new_id
used_ids.add(new_id)There was a problem hiding this comment.
Done, using a remaining_id_counts copy of id_counts decremented per rename, discarding once it hits zero. kinda the same idea as your sketch. Traced through the counter example of ["a10","a10"]+["a"]*11 and confirmed in the code that it correctly produces "a10" as the last id.
| # Map the ID to its next suffix. | ||
| id_suffixes: dict[str, int] = defaultdict(int) | ||
| # Suffix non-unique IDs to make them unique. | ||
| used_ids = all_ids |
There was a problem hiding this comment.
used_ids = all_ids is just an alias of the same set object, so mutations below also mutate all_ids. Prefer a single name (used_ids = set(resolved_ids)), and once you add the remaining-count discard logic this alias becomes even more misleading.
There was a problem hiding this comment.
Fixed, used set(all_ids) rather than set(resolved_ids). Since all_ids is already set(resolved_ids) computed a few lines up (used for the duplicate-count check), and resolved_ids has not been mutated yet at this point, set(all_ids) gives the identical set while avoiding rebuilding it from the last a second time.
Happy to switch it to set(resolved_ids) literally if its preferred to not depend on all_ids being untouched at this point in the function.
2 participants
The Problem
IdMaker.make_unique_parameterset_ids(used to generate node ids for@pytest.mark.parametrize) rebuiltset(resolved_ids)from scratch onevery candidate suffix it tried while de-duplicating colliding ids.
That set-rebuild is O(n), and it sat inside a loop over every parameter
set, making the whole de-duplication step O(n**2) in the number of
parameter sets whenever many of them produce the same id (e.g.
parametrizing with many values that stringify identically, or with
duplicate user-provided ids).
Fix
Build the set of in-use ids once, and update it incrementally as new
ids are assigned, instead of rebuilding it from the list on every
check. This brings the de-duplication step down to O(n).
Testing
make_unique_parameterset_ids()directly with N colliding ids:runtime scaling went from ~4x per doubling of N (quadratic) to ~2x
per doubling (linear), roughly 55x faster at N=8000.
testing/python/metafunc.py::TestMetafunc::test_idmaker_*tests pass unchanged.
testing/python/suite passes (629 passed, 24 skipped, 2 xfailed